Added init_running contract function #536
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Terraform Feature Environment (dev-536)Terraform Initialization ⚙️
|
ppca
reviewed
Mar 27, 2024
contract/src/lib.rs
Outdated
| join_votes: Votes::new(), | ||
| leave_votes: Votes::new(), | ||
| }), | ||
| pending_requests: TreeMap::new(b"m"), |
There was a problem hiding this comment.
what happens to the pending requests in old contract?
There was a problem hiding this comment.
We should not move old requests to the new contract since they will be outdated while you transfer them. Nothing bad will happen if nodes try to push signatures for a non-existent payload.
Votes are not transferred either.
There was a problem hiding this comment.
how would they be outdated? I think it's still fair to maintain these requests though otherwise clients will not be able to tell where their signature went (especially when the NEP lands). We should at the very least report back an error somehow
There was a problem hiding this comment.
They will get an error from the old contract. These requests are short-lived and contract migration happens rarely. Also, nodes will need to change the contract manually and that will take more than 8 seconds. We will have some downtime.
There was a problem hiding this comment.
We may have a more complex design, like a voting mechanism for the "next" contract. But I do not think that is necessary now. This function is mostly for emergencies.
There was a problem hiding this comment.
About moving requests from contract to contract. That is not possible with the current design. We are targeting Sync calls only. Each request must be deleted from the queue in the same sign it was added with. Manually added requests will stay there forever.
There was a problem hiding this comment.
yeah this is fine for now as a emergency lever, but ideally we shouldn't be changing contracts. For the error, I'm talking about not just contract-level errors, but applications that are currently relying on indexer to check whether or not a signature has come in. Those applications will forever wait, which isn't good
There was a problem hiding this comment.
With the current design, nobody will wait for a signature in an async way. Even if we change the design in the future, such applications should have their timeout.
I agree that we should avoid moving from contract to contract since that is a breaking change for all the clients.
We should be able to handle all the breaking changes in the same contract without moving to a new one.
When adding this function, I was mostly thinking about situations, when we lock the contract to make sure it can not be updated. And then if needed, we can move to another contract using this function.
Should we introduce another contract that will redirect sign calls to the current version of the real MPC contract?
We will control such a contract by setting the current MPC contract ID each time we change it. Loosing this key will not introduce any security threats.
At the same time, all versions of the MPC contract will be locked to prevent all sorts of attacks with stolen contract keys.
|
tested on dev. init_running() works |
ppca
approved these changes
Apr 2, 2024
ChaoticTempest
approved these changes
Apr 3, 2024
Terraform Feature Environment Destroy (dev-536)Terraform Initialization ⚙️
|
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
No description provided.